-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
reorganize who knows what about paths #5116
Conversation
@@ -202,8 +202,11 @@ def init_handlers(self, settings): | |||
handlers.extend(load_handlers('services.clusters.handlers')) | |||
handlers.extend(load_handlers('services.sessions.handlers')) | |||
handlers.extend(load_handlers('services.nbconvert.handlers')) | |||
handlers.extend([ | |||
(r"/files/(.*)", AuthenticatedFileHandler, {'path' : settings['notebook_manager'].notebook_dir}), | |||
# FIXME: /files/ should be handled by the Contents service when it exists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the contents web service won't send back raw files - it will send back the JSON model of the file in the same way that GitHub contents or the notebook web service does. I think that means that we can't use the contents web service for /files/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ContentsManager should be able to handle it - it would be a different handler than the '/api/' handler, but the same object turning paths into files. The relevant piece is that contents (generic files) should be responsible rather than the notebook manager, which is currently taking care of both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, yes I agree.
OK I have looked through this and left some inline comments. I have also tested it to make sure the cwd of the kernel is set properly as you walk the dashboard around the directory tree. |
- add NotebookApp.notebook_dir - add KernelManager.root_dir - remove NotebookManager.notebook_dir, move to FileNBM.notebook_dir Default value for KM.root_dir and fNBM.notebook_dir is NotebookApp.notebook_dir, but they can be configured separately. SessionManager passes the API path to KernelManager, which is responsible for turning it into the kernel's cwd.
Should behave more logically (I hope).
Looks good, merging. |
Tested interactively, works fine. Merging. |
reorganize who knows what about paths
reorganize who knows what about paths
(names of the traitlets are still up for discussion)
Setting NotebookApp.notebook_dir sets KM.root_dir and FileNBM.notebook_dir (99% of cases case), but they can be configured independently.
SessionManager passes the API path to KernelManager, which is responsible for turning it into the kernel's cwd.
Finishes up work started in #4925